Skip to content

Add network constraints for fetching notifications with WorkManager#6305

Merged
jmartinesp merged 13 commits intodevelopfrom
misc/check-air-gapped-env-for-workmanager-network-constraints
Mar 10, 2026
Merged

Add network constraints for fetching notifications with WorkManager#6305
jmartinesp merged 13 commits intodevelopfrom
misc/check-air-gapped-env-for-workmanager-network-constraints

Conversation

@jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Mar 9, 2026

Content

  • Add isNetworkBlocked and isInAirGappedEnvironment to NetworkMonitor, which are populated from NetworkCallback. We can remove NetworkBlockedChecker now.
  • Improve the DI of SyncPendingNotificationsRequestBuilder so we use assisted injection now and don't have to add its dependencies to the instantiator.
  • Expose the air-gapped state in EnterpriseService.isInAirGappedEnvironment.
  • Use that to decide if we should add the NetworkCapabilities.NET_CAPABILITY_VALIDATED constraint to the notification fetching workers (which checks Google's 204 endpoints to know if the network has internet connectivity).
  • Add a feature flag to disable this behaviour.

Motivation and context

Part of #6209.

Tests

Things that can be checked:

  • There should be logs for network being disabled when the app goes to background for a while, and of it being enabled when bringing it back to foreground. This means the callback works as the checker did before.
  • If you set up a MITM proxy and block https://www.google.com/generate_204 and http://connectivitycheck.gstatic.com/generate_204, your device will display your network not having internet connectivity, and the app should know it's in an air-gapped env. If using this you receive a notification, it should still be fetched.
  • If for any reason it didn't, disabling the new feature flag and testing it again should make it work as expected.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 16

Checklist

  • Changes have been tested on an Android device or Android emulator with API 24
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • You've made a self review of your PR

@jmartinesp jmartinesp changed the title misc/check-air-gapped-env-for-workmanager-network-constraints Add network constraints to the workers for fetching notifications Mar 9, 2026
@jmartinesp jmartinesp added the PR-Misc For other changes label Mar 9, 2026
@jmartinesp jmartinesp changed the title Add network constraints to the workers for fetching notifications Add network constraints for fetching notifications with WorkManager Mar 9, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/K8HM5t

@codecov
Copy link

codecov bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 74.46809% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.26%. Comparing base (861f268) to head (35ae938).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
...tures/networkmonitor/impl/DefaultNetworkMonitor.kt 0.00% 9 Missing ⚠️
...features/networkmonitor/test/FakeNetworkMonitor.kt 75.00% 1 Missing ⚠️
...kmanager/SyncPendingNotificationsRequestBuilder.kt 95.45% 1 Missing ⚠️
...ager/FakeSyncPendingNotificationsRequestBuilder.kt 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6305      +/-   ##
===========================================
- Coverage    81.27%   81.26%   -0.01%     
===========================================
  Files         2577     2578       +1     
  Lines        70213    70247      +34     
  Branches      9018     9023       +5     
===========================================
+ Hits         57065    57089      +24     
- Misses        9790     9800      +10     
  Partials      3358     3358              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jmartinesp jmartinesp force-pushed the misc/check-air-gapped-env-for-workmanager-network-constraints branch 2 times, most recently from ad00ef6 to 24fa6b3 Compare March 9, 2026 16:17
@jmartinesp jmartinesp marked this pull request as ready for review March 9, 2026 16:19
@jmartinesp jmartinesp requested a review from a team as a code owner March 9, 2026 16:19
@jmartinesp jmartinesp requested review from bmarty and removed request for a team March 9, 2026 16:19
@jmartinesp jmartinesp force-pushed the misc/check-air-gapped-env-for-workmanager-network-constraints branch from 144457b to 03595f2 Compare March 10, 2026 10:28
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, only minor remarks and one question

val isNetworkBlocked: StateFlow<Boolean>

/**
* A flow indicating whether the app is running in an air-gapped environment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* A flow indicating whether the app is running in an air-gapped environment.
* A flow indicating whether the app is running in an air-gapped environment.

workManagerScheduler = workManagerScheduler,
buildVersionSdkIntProvider = buildVersionSdkIntProvider,
resultProcessor = resultProcessor,
syncPendingNotificationsRequestFactory = object : SyncPendingNotificationsRequestBuilder.Factory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a FakeSyncPendingNotificationsRequestFactory class?

@@ -0,0 +1,151 @@
/*
* Copyright (c) 2025 Element Creations Ltd.
* Copyright 2025 New Vector Ltd.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright (c) 2026 Element Creations Ltd. should be enough for new files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh, this is because the file was renamed although git thinks it's new. I'll change it anyway.


override fun isNetworkBlocked(): Boolean = blockedNetworkBlockedChecker.isNetworkBlocked()
override val isNetworkBlocked = MutableStateFlow(false)
override val isInAirGappedEnvironment = MutableStateFlow(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to initialize the 2 stateflow with false? Maybe read values as it was done before in NetworkBlockedChecker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the app is started, there's no chance network can be blocked by Doze, so isNetworkBlocked will always be false. And for the air-gapped env check we need to receive the callback to actually know what's its status, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the app is started, there's no chance network can be blocked by Doze

The app can be started by a Push, so are you sure this is true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good call. It should always be exempted from network restrictions in that case, according to the docs but well... we all know how reliable Google docs are 😅 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's "true" for Firebase, but not really for UnifiedPush...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24ac364 should fix it

…e` for easily creating fakes

Add `FakeSyncPendingNotificationsRequestBuilder`
…onitor.isNetworkBlocked` with its contents

We use a separate component to avoid propagating the `@file:Suppress("DEPRECATION")` to the whole `DefaultNetworkMonitor` file
@sonarqubecloud
Copy link

@jmartinesp jmartinesp enabled auto-merge (squash) March 10, 2026 12:39
@jmartinesp jmartinesp merged commit 912b916 into develop Mar 10, 2026
31 of 32 checks passed
@jmartinesp jmartinesp deleted the misc/check-air-gapped-env-for-workmanager-network-constraints branch March 10, 2026 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-Misc For other changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants